-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Portion of updates go get things building/running with node 19, python3 #118
base: master
Are you sure you want to change the base?
Conversation
volumes: | ||
- ./runcli.js:/app/runcli.js | ||
ports: | ||
- 9229:9229 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compose blame two ports definitions, merged into one. Not sure why 1-to-1 9229 port is required
@@ -1,4 +1,4 @@ | |||
FROM livxtrm/devicefarmer:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't found sources and replaced with upstream
@@ -0,0 +1,50 @@ | |||
class Libusbmuxd < Formula |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy-pasted existing + added depends_on
so pkg-config added to biuld process. Probably fix pc's not needed then(?)
@@ -1,4 +1,4 @@ | |||
#!/usr/bin/python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this path is not available in osx and just fails run
@@ -65,7 +65,7 @@ func cleanup_procs(config *Config) { | |||
}*/ | |||
|
|||
// node --inspect=[ip]:[port] runmod.js device-ios | |||
if cmd[0] == "/usr/local/opt/node@12/bin/node" && cmd[3] == "device-ios" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it better to move to cmd option? coordinator is a go tool, instead of hardcode we can require parameter with a path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cleanup section is an extra protection to doubly sure that old processes are not leftover.
The cmd option itself is supposed to get rid of them all on shutdown, but in some cases ( such as when the coordinator crashes ) it doesn't happen.
o.startFields = log.Fields { | ||
"client_ip": curIP, | ||
"server_ip": serverIP, | ||
"client_hostname": clientHostname, | ||
"server_hostname": serverHostname, | ||
"location": location, | ||
} | ||
o.binary = "/usr/local/opt/node@12/bin/node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess this could be even shitfed into config.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, such things could be shifted into config. I think in this case I did not do so as the proper version and location of node should be detected via additional code instead of being hardcoded.
serverHostname := o.config.Stf.HostName | ||
clientHostname, _ := os.Hostname() | ||
serverIP := o.config.Stf.Ip | ||
|
||
location := fmt.Sprintf("macmini/%s", clientHostname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macmini
is pretty weird hardcode, is it needed for something at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was put here as when this code was used, the names of the machines were not very descriptive, but they were all mac minis. Really this should be a name prefix that can be set if desired in config.
libimobiledevice splitted to glue module that is required for libusbmuxd. ``` brew install --build-from-source --HEAD libimobiledevice.rb brew install --build-from-source --HEAD libimobiledevice-glue.rb brew install --build-from-source --HEAD libusbmuxd.rb ``` Probably better to relax node to just `node` binary, but node 20 had some issues afair Signed-off-by: Kanstantsin Shautsou <[email protected]>
Q about coordinator.go, what is the sense of calling go binary from go binary with json as IPC? Why not call it natively? |
./util/brewser.pl fixpc libusbmuxd 2.0 | ||
#make libimd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo review, also libimd builds fine
Brew link/unlink should handle node switching(?) Signed-off-by: Kanstantsin Shautsou <[email protected]>
It is done because if you call it natively it doesn't work. |
Git submodules will not be used. |
What does adding a go.mod in the root accomplish? |
After going through issues/forks/mrs decided to PR my local changes for building stuff.
libimobiledevice splitted to glue module that is required for libusbmuxd.
Probably better to relax node to just
node
binary, but node 20 had some issues...go stuff is not perfect, i also added go mod in the root of repo, but i'm not sure that it's the best go way.
PS i think switch repos to git submodules would be better and this can allign code/tools in a working state.